Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-46158: Add workflow to perform schema comparisons and check for changes to deployed schemas #277

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Nov 7, 2024

This PR adds a workflow that will check for changes in schemas using the felis diff command. One comparison is performed using the deepdiff comparator, which will show all changes, including ones to metadata fields, e.g., ivoa_ucd. Another comparison step will use alembic, which will only show updates that affect the relational data model.

Finally, a comparison using the alembic mode will be performed on each schema from a list of "deployed" schema YAML files (which is also added by this PR). An error will be raised if any of them have changed (dp02_dc2, etc.).

A preliminary step in the workflow uses git diff to detect which files were changed. Files which were not changed in the repository need not be checked.


Blocked by lsst/felis#109

This is merged now. ^^^^

@JeremyMcCormick JeremyMcCormick changed the title Tickets/dm 46158 DM-46158: Add schema comparison workflow Nov 7, 2024
@JeremyMcCormick JeremyMcCormick marked this pull request as draft November 7, 2024 21:05
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 6 times, most recently from eb88b7e to 80b68f4 Compare November 7, 2024 22:49
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 3 times, most recently from fcba30b to 72a9e39 Compare November 16, 2024 16:10
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 3 times, most recently from 0d2da16 to 56b078f Compare December 5, 2024 00:08
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review December 5, 2024 00:08
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 9 times, most recently from d0b379f to cb91907 Compare December 5, 2024 22:40
@JeremyMcCormick JeremyMcCormick changed the title DM-46158: Add schema comparison workflow DM-46158: Add workflow to perform schema comparisons and check for changes to deployed schemas Dec 6, 2024
Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two significant changes:

  • Trigger only on PRs, and do the diff against the base ref of the PR
  • Modify the .yaml changes on the PR to have one file with a non-schema-changing change to a deployed schema, one with a schema-changing change to a deployed schema, and one with a schema-changing change to a non-deployed schema

Following the next review of this PR:

  • all three .yaml file changes must be removed from the PR before merging.

.github/workflows/compare.yaml Outdated Show resolved Hide resolved
.github/workflows/compare.yaml Outdated Show resolved Hide resolved
.github/workflows/compare.yaml Outdated Show resolved Hide resolved
python/lsst/sdm_schemas/schemas/dp02_dc2.yaml Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 3 times, most recently from 8e21f7c to 319d41a Compare December 9, 2024 19:43
@gpdf gpdf self-requested a review December 10, 2024 00:21
Copy link
Collaborator

@gpdf gpdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Approved assuming a) "DO NOT MERGE" commits (used only for testing) are dropped and b) the commit with the DEPLOYED.txt file is kept separate from the workflow changes.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 3 times, most recently from a99d2d5 to bede84e Compare December 10, 2024 01:18
@JeremyMcCormick
Copy link
Collaborator Author

JeremyMcCormick commented Dec 10, 2024

I've corrected a problem where there was an error if no changes had occurred. And I have updated the workflow so that it skips subsequent skips when there are no changes. I tested these updates for both cases by also re-committing and testing on the dummy schema changes, which I then dropped, again.

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46158 branch 3 times, most recently from 492fe22 to 56930a6 Compare December 10, 2024 01:32
@JeremyMcCormick JeremyMcCormick merged commit 450babd into main Dec 10, 2024
10 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-46158 branch December 10, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants